Fix listing virtual machines with vnfnics - #161
Conversation
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes CloudStack response decoding for the vnfnics field by introducing a structured VnfNic type and updating generated response structs to use it, addressing #159.
Changes:
- Add a hand-maintained
VnfNicstruct to match the (undocumented) API response shape. - Update
vnfnicsfields from[]stringto[]*VnfNicacross multiple response/resource types. - Update generator mapping so
vnfnicsis emitted as[]*VnfNic.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| generate/generate.go | Generates VnfNic and maps vnfnics to []*VnfNic. |
| cloudstack/VirtualNetworkFunctionsService.go | Adds VnfNic type and updates VNF appliance responses to use it. |
| cloudstack/VirtualMachineService.go | Updates many VM-related responses/resources to use []*VnfNic for vnfnics. |
| cloudstack/SnapshotService.go | Updates snapshot response to use []*VnfNic for vnfnics. |
| cloudstack/SSHService.go | Updates SSH reset response to use []*VnfNic for vnfnics. |
| cloudstack/NicService.go | Updates NIC update response to use []*VnfNic for vnfnics. |
| cloudstack/ISOService.go | Updates ISO attach/detach responses to use []*VnfNic for vnfnics. |
| cloudstack/BackupService.go | Updates VM-from-backup response to use []*VnfNic for vnfnics. |
| cloudstack/AffinityGroupService.go | Updates VM affinity group response to use []*VnfNic for vnfnics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Both list structs took their json tag from the API name, so they looked for "vnftemplate" and "vnfappliance". Neither key exists in the server response: ListVnfTemplatesCmd is an empty subclass of ListTemplatesCmd and returns its items under "template", and ListVnfAppliancesCmd inherits execute() from ListVMsCmd and returns its items under "virtualmachine". Both calls therefore returned a correct Count with an always-empty slice, which also made GetVnfTemplateByName/ByID and GetVnfApplianceByName/ByID always report that the resource was not found. Adds both to the existing switch of APIs whose response key differs from the API name, alongside cases like listVirtualMachinesUsageHistory, and regenerates. Verified against a 4.22.1.0 server response that returns one VNF template: it decodes to one element instead of zero.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (1)
generate/generate.go:2432
mapTypemaps any response field namedvnfnics(typelist) to[]*VnfNic, butVnfNicis only emitted opportunistically for thelistVnfAppliancesAPI earlier in this file. This makes the generator brittle: if the API set ever includesvnfnicsbut notlistVnfAppliances(or if services are generated/consumed selectively), the generated code will fail to compile due to an undefinedVnfNictype.
Consider making VnfNic a stable, package-level type that does not depend on a single API being present (e.g., place it in a small hand-maintained file under cloudstack/, or have the generator emit it once when it first encounters a vnfnics list field).
if pName == "vnfnics" {
return "[]*VnfNic"
}
This fixes #159